[WIP] Choice rule payload validation #189

kbrock · 2024-05-22T21:28:32Z

alt to #187

Depend upon:

Overview

Choice does not quite behave the same as the AWS States Language reference implementation.

If you specify a Path that points to nothing or the wrong type, aws raises a States.Runtime. In truth, the type checking is not the most consistent, but the missing path is always raised.

Changes:

Validate Next points to a valid state.
Validate Variable and compare key values are the correct data type.
Raise exceptions when Variable or compare key path values not not found or the wrong data type. It used to throw ruby exceptions.
Fix "IsPresent": true to detect values present rather than not null.
All "Is*" comparisons now support true and false values.
Support Choice with no Default provided.

kbrock · 2024-05-22T23:51:08Z

update:

cop: returning false instead of nil for bad lhv
cop: space in spec
rebased

kbrock · 2024-05-23T00:53:12Z

wip: pre-compiling operations

kbrock · 2024-05-23T04:54:47Z

update:

compiled expression into command tree
changed key comparison to not look for String, but rather (String)(GreaterThan)(Path). It has a few false positives, (IntegerMatchesPath) but they will all probably work.

This is a lot more involved but a lot more strict/stringent

update:

cops: hash rocket, indent, annotation
no longer escaping matches up front (can roll back if others want)

kbrock · 2024-05-23T13:40:44Z

rubocop: command line and web have different opinions on whether a regex is freezable. one complains no matter what code I use here.

un-wip: this seems to work

kbrock · 2024-05-23T19:56:58Z

update:

rebase
simplified down choice class lookup code

update:

added freeze back in

lib/floe/workflow/choice_rule/data.rb

Fryguy · 2024-05-23T20:03:13Z

Can you add specs around these new classes? wondering if would catch the invalid namespace thing I found.

kbrock · 2024-05-23T20:08:13Z

update:

change class reference

I commented out every line there, and the specs fail. So every one of those classes are tested

kbrock · 2024-05-23T20:20:43Z

Discussion:
Instead of using a command pattern, we can change this to store the operation so we wouldn't need all these classes.

e.g.:

OPERATIONS = { "StringEquals" => "==" }
lhs.send(OPERATIONS[choice_value], rhs)

Fryguy · 2024-05-23T21:20:16Z

lib/floe/workflow/choice_rule/data.rb

+            values = COMPARE_RULE.match(key)
+            return [key, DATA_RULES[values[2]], !!values[3]] if values


I think this would read better if the match was decomposed or perhaps if named captures were used instead of using values[2], values[3], etc.

Some other quetsions

where is the prefix used (the (String|Numeric|Boolean|Timestamp) part)? It seems to be ignored here.

The caller does compare_key, klass = klass_params(payload), but this method returns 3 items, so is that a bug? Where did the 3rd param go with the path, and why isn't it being used?

the current pattern for the states/command objects to parse the payload themselves.
Here, we need to parse the string to

The old code ignored the first part, so I continued to ignore it. I can see doing type checks or conversions.

returning 2 vs 3 is probably a bug. thanks

yes, we ignore the type prefix part and don't currently do data type conversion. (not sure if that is needed)

changed this code. think all set.

Fryguy · 2024-05-23T21:24:10Z

Discussion: Instead of using a command pattern, we can change this to store the operation so we wouldn't need all these classes.

e.g.:
OPERATIONS = { "StringEquals" => "==" }
lhs.send(OPERATIONS[choice_value], rhs)

Maybe, but this all feels overly complicated to me personally. I don't see why each of these can't be a simple method (or even just the original code the way it was). Just curious what problem you were trying to solve. I could see creating the classes to encapsulate a true? + valid? pair, but all of the new classes just have a single method.

kbrock · 2024-05-31T02:01:17Z

sorry - this is for previous commit

update:

rebase
dropped comparison classes and put into an operation's hash.
No longer modify parent ChoiceRule class
storing path or value in ref to simplify the initialize code

kbrock · 2024-05-31T02:03:10Z

update

went back to all the is_{type} methods
dropped all separate classes
more pedantic at build time (fewer invalid cases go through

kbrock · 2024-08-01T15:02:34Z

lib/floe/workflow/choice_rule/data.rb

+          return presence_check(context, input) if compare_key == "IsPresent"
+
          lhs = variable_value(context, input)
          rhs = compare_value(context, input)

-          validate!(lhs)
-


Before:

fetches a value, and verifies what ever came out is not null

After:

fetches a value and verifies it existed in the input hash

kbrock · 2024-08-01T15:04:26Z

lib/floe/workflow/choice_rule/data.rb

-          when "IsTimestamp" then is_timestamp?(lhs)
+          when "IsNull" then is_null?(lhs, rhs)
+          when "IsNumeric" then is_numeric?(lhs, rhs)
+          when "IsString" then is_string?(lhs, rhs)


Before: "IsString" => "*" verified it is a string.
After: "IsString" => true works that way, but "IsString" => false does the opposite
(across all checks)

kbrock · 2024-08-01T15:07:56Z

lib/floe/workflow/choice_rule/data.rb

+        def variable_value(context, input)
+          fetch_path("Variable", variable, context, input)
        end


(moved over from ChoiceRule since this is data specific)
Before:

convert @variable to a Path at runtime and fetch value.
After:

@variable is a Path`, so bad Path detected at initialization time

Path not present in input is a runtime error

Data type of input is checked.

kbrock · 2024-08-01T15:11:12Z

lib/floe/workflow/choice_rule/data.rb

+        def parse_compare_key
+          @compare_key = payload.keys.detect { |key| key.match?(TYPE_CHECK) || key.match?(OPERATION) }
+          parser_error!("requires a compare key") unless compare_key
+
+          @type, _operator, @path = OPERATION.match(compare_key)&.captures
+          # TYPE_CHECK doesn't match this regex, so @path = @type = nil
+          # @path.nil? means this the compare_value will always be a static value (true or false)
+          # @type.nil? means we won't type check the variable or compare value
+        end


Yea, this got more complicated.

After:

Can detect bad compare values at initialization time.

Can detect bad paths at initialization time.

Store type checking information (only for operations. bad type in the type check are informational (true false) rather than errors.

kbrock · 2024-08-01T15:12:33Z

lib/floe/workflow/path.rb

-        results.count < 2 ? results.first : results
+        case results.count
+        when 0
+          raise Floe::PathError, "Path [#{payload}] references an invalid value"


This one change has big consequences.

Before: A path pointing to nothing would just treat the value as nil
After: A path pointing to nothing is a runtime error

kbrock · 2024-08-01T15:15:37Z

lib/floe/workflow/state.rb

+      rescue Floe::ExecutionError => e
+        mark_error(context, e)


This was detected because tests running against a state would work, but running against a workflow would not.

Before:

Errors needed to be caught by each State

Errors resulted in a state that was not complete.

Errors did not always get their way into output.

After:

runtime errors always set the context accordingly.

kbrock · 2024-08-01T15:21:51Z

lib/floe/workflow/state.rb

+      def finish(context)
+        mark_finished(context)
+      end


The code for finish hasn't changed, it just now resides in mark_finished.

Reasoning: If State throws an error, it almost always threw it in finish. When an error is thrown, super.finish is not called. This means no logging and FinishedTime is never set -- so the state looks like it is not finished.

Now, we catch errors thrown, and ensure super.finish is called. Easiest way to do this is to move that code into a separate method -- mark_finish.

kbrock · 2024-08-09T16:00:25Z

WIP: piece by piece pulling out PRs from this

kbrock · 2024-10-15T20:43:57Z

update:

fix guard clause - ensured bad operators found in initializer not runtime

update:

fix } spacing cop

miq-bot · 2024-11-06T19:40:44Z

Checked commit kbrock@a786b40 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 🍰

miq-bot · 2024-12-09T22:20:42Z

This pull request is not mergeable. Please rebase and repush.

kbrock added the enhancement New feature or request label May 22, 2024

kbrock requested review from agrare and Fryguy as code owners May 22, 2024 21:28

kbrock changed the title ~~Choice rule payload validation~~ [WIP] Choice rule payload validation May 23, 2024

miq-bot added the wip label May 23, 2024

kbrock force-pushed the choice_rule_payload_validation branch from a0d0c26 to 0993b77 Compare May 23, 2024 04:47

kbrock force-pushed the choice_rule_payload_validation branch from 0993b77 to 07a1411 Compare May 23, 2024 04:58

kbrock changed the title ~~[WIP] Choice rule payload validation~~ Choice rule payload validation May 23, 2024

miq-bot removed the wip label May 23, 2024

kbrock force-pushed the choice_rule_payload_validation branch 3 times, most recently from fdab00e to e1bce92 Compare May 23, 2024 19:55

Fryguy reviewed May 23, 2024

View reviewed changes

lib/floe/workflow/choice_rule/data.rb Outdated Show resolved Hide resolved

kbrock force-pushed the choice_rule_payload_validation branch from e1bce92 to 4ff6108 Compare May 23, 2024 20:07

Fryguy reviewed May 23, 2024

View reviewed changes

kbrock changed the title ~~Choice rule payload validation~~ [WIP] Choice rule payload validation May 24, 2024

kbrock added the wip label May 24, 2024

kbrock force-pushed the choice_rule_payload_validation branch 2 times, most recently from 856fce4 to 1ceb8f4 Compare May 31, 2024 02:01

kbrock commented Aug 1, 2024

View reviewed changes

kbrock mentioned this pull request Aug 1, 2024

Choice rule payload validation path #253

Merged

kbrock force-pushed the choice_rule_payload_validation branch from 893c61d to 1dfe07e Compare August 2, 2024 21:05

miq-bot added the unmergeable label Aug 8, 2024

kbrock added the wip label Aug 9, 2024

kbrock changed the title ~~Choice rule payload validation~~ [WIP] Choice rule payload validation Aug 9, 2024

kbrock force-pushed the choice_rule_payload_validation branch from 1dfe07e to b60a866 Compare August 21, 2024 14:18

kbrock removed the unmergeable label Aug 21, 2024

kbrock force-pushed the choice_rule_payload_validation branch from b60a866 to 44a8a3e Compare August 23, 2024 20:59

miq-bot added the unmergeable label Aug 27, 2024

kbrock force-pushed the choice_rule_payload_validation branch 3 times, most recently from 1f374b2 to 13a4d7c Compare October 15, 2024 20:43

miq-bot removed the unmergeable label Oct 17, 2024

kbrock mentioned this pull request Nov 6, 2024

Use send for choice operations #295

Merged

miq-bot added the unmergeable label Nov 6, 2024

Lookup for operations

a786b40

kbrock force-pushed the choice_rule_payload_validation branch from 13a4d7c to a786b40 Compare November 6, 2024 19:37

miq-bot removed the unmergeable label Nov 21, 2024

miq-bot added the unmergeable label Dec 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Choice rule payload validation #189

[WIP] Choice rule payload validation #189

kbrock commented May 22, 2024 •

edited

Loading

kbrock commented May 22, 2024

kbrock commented May 23, 2024

kbrock commented May 23, 2024 •

edited

Loading

kbrock commented May 23, 2024 •

edited

Loading

kbrock commented May 23, 2024

Fryguy commented May 23, 2024

kbrock commented May 23, 2024

kbrock commented May 23, 2024

Fryguy May 23, 2024

kbrock May 24, 2024

kbrock May 31, 2024

Fryguy commented May 23, 2024

kbrock commented May 31, 2024 •

edited

Loading

kbrock commented May 31, 2024

kbrock Aug 1, 2024

kbrock Aug 1, 2024

kbrock Aug 1, 2024

kbrock Aug 1, 2024

kbrock Aug 1, 2024

kbrock Aug 1, 2024

kbrock Aug 1, 2024

kbrock commented Aug 9, 2024

kbrock commented Oct 15, 2024

miq-bot commented Nov 6, 2024

miq-bot commented Dec 9, 2024

		values = COMPARE_RULE.match(key)
		return [key, DATA_RULES[values[2]], !!values[3]] if values

[WIP] Choice rule payload validation #189

Are you sure you want to change the base?

[WIP] Choice rule payload validation #189

Conversation

kbrock commented May 22, 2024 • edited Loading

Overview

kbrock commented May 22, 2024

kbrock commented May 23, 2024

kbrock commented May 23, 2024 • edited Loading

kbrock commented May 23, 2024 • edited Loading

kbrock commented May 23, 2024

Fryguy commented May 23, 2024

kbrock commented May 23, 2024

kbrock commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented May 23, 2024

kbrock commented May 31, 2024 • edited Loading

kbrock commented May 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock commented Aug 9, 2024

kbrock commented Oct 15, 2024

miq-bot commented Nov 6, 2024

miq-bot commented Dec 9, 2024

kbrock commented May 22, 2024 •

edited

Loading

kbrock commented May 23, 2024 •

edited

Loading

kbrock commented May 23, 2024 •

edited

Loading

kbrock commented May 31, 2024 •

edited

Loading